Skip to content

smartcontract: enforce Permission flags on multicast subscribe and create-user handlers#3966

Merged
bgm-malbeclabs merged 2 commits into
mainfrom
feature/least-privilege-oracle-access
Jul 9, 2026
Merged

smartcontract: enforce Permission flags on multicast subscribe and create-user handlers#3966
bgm-malbeclabs merged 2 commits into
mainfrom
feature/least-privilege-oracle-access

Conversation

@bgm-malbeclabs

@bgm-malbeclabs bgm-malbeclabs commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary of Changes

  • Completes the least-privilege (RFC-2) Permission-account migration for the two remaining serviceability handlers the feed oracle calls, so the oracle can operate on an ACCESS_PASS_ADMIN | USER_ADMIN Permission account instead of foundation_allowlist membership — least privilege for a hot, always-online key.
  • UpdateMulticastGroupRoles: granting roles on another owner's pass is now an ACCESS_PASS_ADMIN operation, while removal-only cleanup stays USER_ADMIN (unchanged from [New Permission 3/5] smartcontract: enforce Permission-based authorization in existing instructions #3206). Splitting by operation preserves the deliberate "USER_ADMIN is removal-only here" contract and the exact historical NotAllowed / Unauthorized error variants.
  • CreateSubscribeUser owner-override now accepts a USER_ADMIN Permission holder in addition to the sentinel and foundation. The Permission account is threaded through CreateUserCoreAccounts (None for plain CreateUser, which never overrides the owner).
  • Both handlers disambiguate the optional trailing Permission account from the EdgeSeat feed/device accounts via split_trailing_permission (PDA match, not position), so Permission and Feed coexist unambiguously. This also fixes a latent ordering bug in create_subscribe's EdgeSeat feed wiring: the SDK's assemble_instructions always appends [payer, system, permission?] last, so the feed sits before payer/system, not after — only latent today because feed_pk is None everywhere.
  • The Permission path carries no owns-it restriction, so a flag holder acts across owners — required for the validator-owned users and passes the oracle manages. The change is additive: every existing caller keeps its exact current authority.
  • permission/audit.rs: corrects the NON_MIGRATED_SUBSYSTEMS text — the user-create owner override now honors USER_ADMIN via authorize() (foundation via the legacy mapping, Permission holders directly), leaving only the sentinel direct check un-migrated.

Context: RFC-2 (malbeclabs/infra#1764), malbeclabs/doublezero-shreds#517. The subscriber-allowlist handlers (Add/Remove MulticastGroupSubAllowlist) were migrated to authorize() upstream in #3982 while this branch was in review, so that half is dropped here; this PR completes the set for the subscribe/create-user surface, following the established authorize() + split_trailing_permission pattern with no new authorization helper.

Diff Breakdown

Category Files Lines (+/-) Net
Tests 4 +550 / -3 +547
Docs 2 +26 / -2 +24
Core logic 3 +87 / -36 +51
Scaffolding 2 +7 / -2 +5
Total 11 +670 / -43 +627

Overwhelmingly tests: the authorization logic itself is a small, additive change to three processors, with the bulk of the diff proving the new cross-owner Permission paths.

Key files (click to expand)

Testing Verification

  • Added integration tests exercising the new Permission path for each wired handler, all proving the cross-owner (no owns-it) property:
    • test_subscribe_access_pass_admin_permission_allowed — an ACCESS_PASS_ADMIN holder (not owner/foundation/sentinel/feed) subscribes a user cross-owner.
    • test_create_subscribe_user_user_admin_owner_override — a USER_ADMIN holder creates a subscribe-user with owner != payer.
    • Preserved the [New Permission 3/5] smartcontract: enforce Permission-based authorization in existing instructions #3206 contract tests: test_subscribe_user_admin_permission_rejected (USER_ADMIN still cannot add roles) and test_unsubscribe_user_admin_permission_allowed (USER_ADMIN removal cleanup).
  • Retained ACCESS_PASS_ADMIN coverage for the subscriber-allowlist path (test_multicast_subscriber_allowlist_access_pass_admin_permission) — the oracle's exact path, which serviceability: authorize multicast instructions via Permission accounts #3982 migrated without adding a test.
  • Updated feed_metro_gate_test to the SDK-consistent account order (feed before payer/system); all 4 EdgeSeat metro-gate cases pass, confirming feed + the new Permission parsing coexist.
  • Note: test_set_accesspass_refills_depleted_user_payer is a pre-existing failure on main (airdrop-refill balance assertion, unrelated to authorization) — confirmed it fails identically on a clean origin/main checkout.

juan-malbeclabs

This comment was marked as spam.

@juan-malbeclabs juan-malbeclabs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The authorization logic itself is solid: the migration is faithful to the authorize() pattern, the error-variant preservation (NotAllowed vs Unauthorized) in UpdateMulticastGroupRoles is careful, and split_trailing_permission correctly disambiguates all four possible account layouts. The tests prove the cross-owner property and preserve the #3206 contracts.

The blocker is integration, not logic. The branch is based on a merge-base that predates EdgeSeat (#3955 / 0e488f63, already on main) — I confirmed the branch does not contain those commits. Two of the migrated handlers parse their trailing accounts positionally, and that collides with the EdgeSeat metro-gate trailing accounts once rebased. The merge-base diff hides this because they are additions in adjacent regions that git merges textually while producing incorrect semantics.

Each point is detailed inline with a proposed fix. Summary:

  • 🔴 High — trailing-account collision with EdgeSeat in create_subscribe.rs and subscribe.rs after rebase.
  • 🟡 MediumNON_MIGRATED_SUBSYSTEMS in smartcontract/cli/src/permission/audit.rs is now stale (CLAUDE.md / Permissions rule). The file is not in the diff, so I commented on create_core.rs.
  • 🟢 Nit — missing coverage for the n=4 branch of split_trailing_permission (user_payer + permission together).

Recommendation: rebase onto main and re-verify end-to-end before merging; port the two handlers to the PDA-match approach already used in add.rs. The green suite reported in the PR was necessarily run against the stale base and does not cover this interaction.

…eate-user handlers

Continues the Permission-account authorization migration to the remaining
multicast/user handlers the feed oracle calls, so the oracle can operate on an
ACCESS_PASS_ADMIN | USER_ADMIN Permission account instead of foundation_allowlist
membership (least privilege for a hot, always-online key).

- UpdateMulticastGroupRoles: granting roles on another owner's pass is now an
  ACCESS_PASS_ADMIN operation; removal-only cleanup stays USER_ADMIN. The
  historical NotAllowed/Unauthorized error variants are preserved.
- CreateSubscribeUser owner-override now accepts a USER_ADMIN Permission holder
  in addition to the sentinel and foundation, threaded through create_user_core.
- Both handlers disambiguate the optional trailing Permission account from the
  EdgeSeat feed/device accounts via split_trailing_permission (PDA match, not
  position), so Permission and Feed coexist unambiguously. This also fixes a
  latent ordering bug in create_subscribe's EdgeSeat feed wiring: the SDK's
  assemble_instructions always appends [payer, system, permission] last, so the
  feed sits before payer/system, not after (feed_metro_gate_test updated to the
  SDK-consistent order).
- permission/audit.rs: NON_MIGRATED_SUBSYSTEMS text updated -- the user-create
  owner override now honors USER_ADMIN via authorize(), leaving only the
  sentinel direct check un-migrated.
- Adds ACCESS_PASS_ADMIN coverage for the subscriber-allowlist path (the
  processors themselves were migrated upstream in #3982).
- PERMISSION.md: document domain-instruction enforcement for the multicast /
  user surface, and correct legacy-mapping rows that were stale after the
  activator retirement.

The Permission path carries no owns-it restriction, so a flag holder acts across
owners, which the oracle needs for validator-owned users and passes. The change
is additive: every existing caller keeps its current authority.

Refs: RFC-2 (malbeclabs/infra#1764), malbeclabs/doublezero-shreds#517
@bgm-malbeclabs bgm-malbeclabs force-pushed the feature/least-privilege-oracle-access branch from b62f49d to 8061a23 Compare July 9, 2026 18:24
@bgm-malbeclabs bgm-malbeclabs changed the title smartcontract: enforce Permission flags on multicast allowlist and subscribe/create-user handlers smartcontract: enforce Permission flags on multicast subscribe and create-user handlers Jul 9, 2026

@juan-malbeclabs juan-malbeclabs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All review points addressed in 247ea4a and verified in the tree:

  • 🔴 create_subscribe.rs and subscribe.rs: ported to split_trailing_permission; the Permission is disambiguated by PDA and Feed/device coexist without collision.
  • 🟡 audit.rs: NON_MIGRATED_SUBSYSTEMS updated — the owner override now honors USER_ADMIN via authorize(), leaving only the direct sentinel_authority check un-migrated.
  • 🟢 add.rs: took main's version (#3982) during the rebase.
  • 🟢 n=4 coverage: accepted via the helper's unit tests.

LGTM.

@bgm-malbeclabs bgm-malbeclabs merged commit acf3d73 into main Jul 9, 2026
47 of 50 checks passed
@bgm-malbeclabs bgm-malbeclabs deleted the feature/least-privilege-oracle-access branch July 9, 2026 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants